Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update did:jwk resolve test vectors #69

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Conversation

mistermoe
Copy link
Contributor

  • Added README to describe inputs and outputs of did:jwk test vectors
  • Removed didResolutionResult from output. Inclusion of this property left room to believe that the resolve implementations should contain a literal didResolutionResult property
  • added didDocumentMetadata to output where missing. Note: on success, this will always be an empty object for did:jwk
  • added didResolutionMetadata to output where missing. Note: on success, this will always be an empty object for did:jwk

Copy link
Collaborator

@amika-sq amika-sq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a comment (non blocking) - I would recommend adding vectors for all the key type/algs we support. I've run into issues by assuming these work and realizing much later they in fact don't. Forcing compatibility with each support key type here will prevent that risk.

@mistermoe
Copy link
Contributor Author

great point @decentralgabe we should definitely do that. will update the PR to reflect

@frankhinek
Copy link
Contributor

frankhinek commented Nov 29, 2023

just a comment (non blocking) - I would recommend adding vectors for all the key type/algs we support. I've run into issues by assuming these work and realizing much later they in fact don't. Forcing compatibility with each support key type here will prevent that risk.

Great point. One thing to note is that we're not yet at a point of consistent support for crypto algs across the web5-* libs:

  • web5-js and web5-kt support secp256k1
  • web5-js and web5-kt haven't added support for secp256r1 (aka P-256) yet
  • web5-js supports X25519 but web5-kt doesn't yet
  • web5-rs is quickly evolving but I think it supports secp256k1, secp256r1, and Ed25519 but not yet X25519

We could always expand the did:jwk test vector to include what should be supported and not run the vector for all SDKs until their crypto packages are fully compliant.

@frankhinek
Copy link
Contributor

frankhinek commented Nov 29, 2023

Do we want to consider canonicalizing the public key before Base64URL encoding?

per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

@andresuribe87
Copy link
Contributor

Do we want to consider canonicalizing the public key before Base64URL encoding?

per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

@frankhinek
Copy link
Contributor

Do we want to consider canonicalizing the public key before Base64URL encoding?
per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

It is but this PR's resolution results are based on not-canonicalizing the public key. As a result, if a web5-* implementation does canonicalization it will fail this vector.

@andresuribe87
Copy link
Contributor

Do we want to consider canonicalizing the public key before Base64URL encoding?
per the spec it is optional:

Canonicalization such as JCS is not required but may be helpful if the resulting DID isn't going to be stored in its serialized form.

I thought that would be part of creation, but I believe this PR is only for resolution

It is but this PR's resolution results are based on not-canonicalizing the public key. As a result, if a web5-* implementation does canonicalization it will fail this vector.

I'm a bit confused still, hoping you can help me understand.

Are you suggesting that resolution should canonicalize the encoded JWK before putting it in the did document? I.e. something like the following

resolve("did:jwk:<foo>").didDocument.verificationMethod[0].publicKeyJwk == canonicalize(decode64url("<foo>"))

@decentralgabe
Copy link
Member

Canonicalization was removed from the latest draft, so I would advise against it https://github.com/quartzjer/did-jwk/pull/21/files

@frankhinek
Copy link
Contributor

Canonicalization was removed from the latest draft, so I would advise against it https://github.com/quartzjer/did-jwk/pull/21/files

Excellent insight! Thanks @decentralgabe. Makes the decision easy.

@andresuribe87 andresuribe87 mentioned this pull request Nov 29, 2023
@mistermoe
Copy link
Contributor Author

updated vectors to include supported cryptographic algorithms - secp256k1 & Ed25519

@mistermoe mistermoe merged commit faab9f2 into main Dec 4, 2023
6 checks passed
@mistermoe mistermoe deleted the fix/did-jwk-resolve-vectors branch December 4, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants